Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add codegen filter to ModelV2 #7898

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Add codegen filter to ModelV2 #7898

merged 1 commit into from
Sep 4, 2024

Conversation

leovalais
Copy link
Contributor

@leovalais leovalais commented Jul 1, 2024 •
edited
Loading

Adds an option to parametrize which implementation ModelV2 has to generate. This is required to support single PK field models (such as Timetable).

No impact on compilation times.

refs #8111

@leovalais leovalais force-pushed the lva/modelv2-selective-impls branch from 590c43f to 19a2ddb Compare July 16, 2024 20:48
@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2024 •
edited
Loading

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 96.01770% with 9 lines in your changes missing coverage. Please review.

Project coverage is 36.95%. Comparing base (5038159) to head (3e58cbb).
Report is 4 commits behind head on dev.

Files with missing lines Patch % Lines
editoast/editoast_derive/src/modelv2/args.rs 0.00% 4 Missing ⚠️
editoast/editoast_derive/src/modelv2/parsing.rs 70.00% 3 Missing ⚠️
editoast/editoast_derive/src/modelv2/crud.rs 98.79% 1 Missing ⚠️
editoast/src/modelsv2/rolling_stock_livery.rs 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@             Coverage Diff              @@
##                dev    #7898      +/-   ##
============================================
+ Coverage     36.88%   36.95%   +0.07%     
  Complexity     2213     2213              
============================================
  Files          1253     1254       +1     
  Lines        113838   113987     +149     
  Branches       3185     3185              
============================================
+ Hits          41991    42128     +137     
- Misses        69955    69967      +12     
  Partials       1892     1892
Flag Coverage Δ
core 74.75% <ø> (ø)
editoast 72.05% <96.01%> (+0.11%) ⬆️
front 14.94% <ø> (ø)
gateway 2.20% <ø> (ø)
osrdyne 2.71% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 86.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leovalais leovalais mentioned this pull request Jul 18, 2024
Open
@leovalais leovalais force-pushed the lva/modelv2-selective-impls branch 3 times, most recently from 14a7f6f to 7ba0bce Compare September 2, 2024 09:25
@leovalais leovalais changed the title Add codegen filter to ModelV2 (maybe) Add codegen filter to ModelV2 Sep 2, 2024
@leovalais leovalais marked this pull request as ready for review September 2, 2024 09:26
@leovalais leovalais requested a review from a team as a code owner September 2, 2024 09:26
flomonster
flomonster approved these changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the fact that we haven't seen any improvement in compilation time seems to indicate that ModelV2 may not be the source of the problem.

editoast/editoast_derive/src/modelv2/args.rs Outdated Show resolved Hide resolved
@leovalais leovalais force-pushed the lva/modelv2-selective-impls branch 2 times, most recently from 4ee7ddc to c34c7b5 Compare September 3, 2024 13:02
Castavo
Castavo approved these changes Sep 3, 2024
View reviewed changes
editoast/src/modelsv2/documents.rs Outdated Show resolved Hide resolved
editoast/editoast_derive/src/modelv2/args.rs Outdated Show resolved Hide resolved
woshilapin
woshilapin approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok but as stated by @flomonster, if we don't see any compilation improvement, do we need the additional complexity?

@leovalais
Copy link
Contributor Author

@woshilapin I drafted this PR a while ago hoping it would help with compilation times. It didn't help so I abandoned the idea. However @Castavo noticed a problem with models having only a single primary key (such as Timetable 👀) for which the ModelV2 traits are currently implemented by hand. Being able to select which operations we choose to generate will be necessary to support this case => #8111. Whether we want to provide a user-facing api for ImplPlan is another debatable question, but we need the internals.

@leovalais leovalais force-pushed the lva/modelv2-selective-impls branch 3 times, most recently from 30ede7f to 83ff709 Compare September 4, 2024 11:11
@leovalais leovalais added this pull request to the merge queue Sep 4, 2024
Merged via the queue into dev with commit 7ba77b2 Sep 4, 2024
22 checks passed
@leovalais leovalais deleted the lva/modelv2-selective-impls branch September 4, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flomonster flomonster flomonster approved these changes

@woshilapin woshilapin woshilapin approved these changes

@Castavo Castavo Castavo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@leovalais @codecov-commenter @woshilapin @flomonster @Castavo